Otel profiles input package support#1092
Conversation
teresaromero
left a comment
There was a problem hiding this comment.
👍🏻
nit: not sure if we can use just one changelog entry. i guess it was divided to add clear changes on inputs and datastreams
| - description: Add support for auto_expand_replicas, max_result_window, and refresh_interval index settings in data stream templates. | ||
| type: enhancement | ||
| link: https://github.com/elastic/package-spec/pull/1092 | ||
| - description: Add support for `profiles` policy template type in input packages | ||
| type: enhancement | ||
| link: https://github.com/elastic/package-spec/pull/1092 |
There was a problem hiding this comment.
Does these new fields depend on any other Kibana pull request ?
If so, it could be interesting to add it here the link to those as a comment in the changelog.
# Pending on https://github.com/elastic/kibana/issues/x
- description: Add support for auto_expand_replicas, max_result_window, and refresh_interval index settings in data stream templates.
type: enhancement
link: https://github.com/elastic/package-spec/pull/1092
- description: Add support for `profiles` policy template type in input packages
type: enhancement
link: https://github.com/elastic/package-spec/pull/1092 | - logs | ||
| - synthetics | ||
| - traces | ||
| - profiles |
There was a problem hiding this comment.
In data streams (integration packages) , there is a profiling type.
package-spec/spec/integration/data_stream/manifest.spec.yml
Lines 546 to 556 in f0fe763
What is the one that should be used ? profiles or profiling ?
There was a problem hiding this comment.
There was a problem hiding this comment.
It looks like there is some level of support in Fleet https://github.com/elastic/kibana/blob/9faf9886e6c1be2a5bab49cd084c7ef4328161ad/x-pack/platform/plugins/shared/fleet/common/types/models/data_stream.ts#L28
Though I don't think we have ever used it, so it'd be good to double-check.
What we are likely missing is support for OTel. For other signal types we add transforms for dynamic routing, and some other Fleet-specific settings. This will need to be added, around here: https://github.com/elastic/kibana/blob/9faf9886e6c1be2a5bab49cd084c7ef4328161ad/x-pack/platform/plugins/shared/fleet/server/services/agent_policies/otel_collector.ts#L122
@MichelLosier could you add a compliance test? It would be similar to this one, but for profiling
There was a problem hiding this comment.
I think I misinterpreted the original ask, although profiles is the OTEL type, profiling is what ES expects and what we'll want to use instead. Important too since the policy_template type can be used to set the data_stream type. I'll update this.
This seems to be the prior kibana support?:
elastic/kibana#155115
There was a problem hiding this comment.
I just noticed: https://github.com/elastic/prodfiler/issues/6646
I'll need to step back on this one and regather context on the intent here with the type value we want to support.
I'm going to put this into draft for now, thank you for the initial reviews
There was a problem hiding this comment.
In data streams (integration packages) , there is a profiling type.
[..]
do you know if that profiling type was ever used or supported in Kibana ?
The profiling team is using a custom Elasticsearch plugin to set up and manage indices/data streams. And as far as I know, there is no way to replicate something similar in an integration package - and there is no plan to do so. Maybe profiling related indices/data streams will be set up in the future using an integration and avoiding the Elasticsearch plugin - but this needs more investigation, testing, planing and also a migration story.
For the time being, https://github.com/elastic/prodfiler/issues/6641 has priority for us, so that we can provide an integration for the upstream OTel project eBPF profiler. For this, indices/datastreams are set up via the Elasticsearch profiling plugin - no changes on this.
So the change can be limited to add profiles as a policy_template.type for input packages.
There was a problem hiding this comment.
Thank you, this is very helpful context!
There was a problem hiding this comment.
@jsoriano
Added the kibana changes here: elastic/kibana#254090
For the compliance test if I have the test package format_version at 3.6.0 it fails on the spec its testing against not being released. If I switch to 3.5.0 it passes - is this a change I should keep locally for development or should I have the test package at 3.6.0?
There was a problem hiding this comment.
If this new type requires changes in kibana to be included in 9.4 , it could be added a JSON patch to remove that profiles item for spec < 3.6.0 as part of these changes:
Adds support for the `profiles` signal type when creating otel transforms Relates to: elastic/prodfiler#6641 Package spec changes: elastic/package-spec#1092
| @3.6.0 | ||
| Scenario: OTel input package with profiles type can be installed | ||
| Given the "good_input_profiles" package is installed | ||
| And a policy is created with "good_input_profiles" package, "0.0.1" version, "profilingreceiver" template, "profilingreceiver" input, "otelcol" input type and dataset "spec.otel_input_test" |
There was a problem hiding this comment.
I think it is missing the Then section to validate that the index template is created.
| @3.6.0 | |
| Scenario: OTel input package with profiles type can be installed | |
| Given the "good_input_profiles" package is installed | |
| And a policy is created with "good_input_profiles" package, "0.0.1" version, "profilingreceiver" template, "profilingreceiver" input, "otelcol" input type and dataset "spec.otel_input_test" | |
| @3.6.0 | |
| Scenario: OTel input package with profiles type can be installed | |
| Given the "good_input_profiles" package is installed | |
| And a policy is created with "good_input_profiles" package, "0.0.1" version, "profilingreceiver" template, "profilingreceiver" input, "otelcol" input type and dataset "spec.otel_input_test" | |
| Then there is an index template "profiles-spec.otel_input_test" with pattern "profiles-spec.otel_input_test.otel-*" |
There was a problem hiding this comment.
I did have this but removed it per @florianl 's comment that it misrepresents a bit the usage of the profiles input package, since index templates will be setup by the profiling package separately and not described through the input package in this way. So based on that it felt like asserting on the policy creation felt like enough.
In any I do see the value in despite of that, of having the assertion on having an index template created just to confirm that functionality still works with this type. WDYT?
There was a problem hiding this comment.
But I think that the comment was more related to the index template settings in the manifest, not about the assertions in the compliance test.
If there is no Then in this compliance test, it just installs the package but it does not perform any validation on that. I think it could be still be tested that if the package is installed with the specified variables (And line), an index template with a given pattern is going to be installed. Is that the expected behavior?
Could you try to install that test package (it could be done via API) and create a policy in Kibana ? To validate that the index template is created.
There was a problem hiding this comment.
But I think that the comment was more related to the index template settings in the manifest, not about the assertions in the compliance test.
Gotcha, would you be able to clarify here @florianl ?
As part of my read was also this comment
The package is intended to "just" add the EDOT configuration - see agent/input/input.yml.hbs. If and how datastreams can be used is still under discussion as it has to work with the Profiling plugin in ES.
As I understand it, the intent here is to have the input package setup the EDOT config and inputs, but sounds like there are still unknowns on whether the package should setup any ES assets, at least for now. I think adding support for the missing index settings adds support for that exploration in the meantime until its understood how the profiling plugin in ES, which does some of this work separately, works with or against the package install behavior.
If its the case we don't want to create an index template here, we could setup a new assertion for compliance tests that validates the expected otel config. Here is what is produced from this package currently:
inputs: []
receivers:
profiling/profilingreceiver-otelcol-otelcol-good_input_profiles-profilingreceiver:
samples_per_second: <SAMPLES_PER_SECOND>
service:
pipelines:
profiles/profilingreceiver-otelcol-otelcol-good_input_profiles-profilingreceiver:
receivers:
- >-
profiling/profilingreceiver-otelcol-otelcol-good_input_profiles-profilingreceiver
processors:
- >-
transform/profilingreceiver-otelcol-otelcol-good_input_profiles-profilingreceiver-routing
processors:
transform/profilingreceiver-otelcol-otelcol-good_input_profiles-profilingreceiver-routing:
profile_statements:
- context: profile
statements:
- set(attributes["data_stream.type"], "profiles")
- set(attributes["data_stream.dataset"], "profilingreceiver")
- set(attributes["data_stream.namespace"], "default")
Could you try to install that test package (it could be done via API) and create a policy in Kibana ? To validate that the index template is created.
When installed in its current state, no index template is produced. If I include an elasticsearch.index_template definition we have one created.
There was a problem hiding this comment.
But I think that the comment was more related to the index template settings in the manifest, not about the assertions in the compliance test.
Gotcha, would you be able to clarify here @florianl ?
With #1092 (comment) I want to avoid conflicts of expectations. elasticsearch.index_template will currently not work and if they are used, they will conflicts with the ES plugin for profiling. Therefore, I think they should not be included in the good_input_profiles example.
As I'm not an expert on the compliance test, I can't speak for that.
[..] sounds like there are still unknowns on whether the package should setup any ES assets, at least for now.
The package should not and is not intended to, set up any ES assets as they would conflicts with the ES plugin for profiling. Once the ES plugin for profiling is replaced with something else (some time in the far future, as there are no ideas or plans for it), this could change.
When installed in its current state, no index template is produced.
That is the expected outcome for the profiling team.
There was a problem hiding this comment.
IIUC then this new input package type profiles just is intended to be used in OTel input packages, is that right ? That would mean that this profiles should be just available if the otelcol input type is set in the policy template.
Is my understanding correct ? @MichelLosier @kpollich @florianl
If it is like that, it would be needed to update the JSON schema (or add a new semantic validation rule) to disallow setting profiles in those cases that the input is not otelcol to avoid any issues. And it would be good also to add a new test "bad" package where the profiles is used with an input like metrics or logs.
I tried to apply this into the JSON schema, this could make work
--- spec/input/manifest.spec.yml
+++ spec/input/manifest.spec.yml
@@ -94,27 +94,38 @@ spec:
$ref: "../integration/manifest.spec.yml#/definitions/dynamic_signal_types"
deprecated:
$ref: "../integration/manifest.spec.yml#/definitions/deprecated"
- oneOf:
- - required:
- - name
- - title
- - description
- - type
- - input
- - template_path
- not:
- required:
- - template_paths
- - required:
- - name
- - title
- - description
- - type
- - input
- - template_paths
- not:
- required:
+ allOf:
+ - oneOf:
+ - required:
+ - name
+ - title
+ - description
+ - type
+ - input
- template_path
+ not:
+ required:
+ - template_paths
+ - required:
+ - name
+ - title
+ - description
+ - type
+ - input
+ - template_paths
+ not:
+ required:
+ - template_path
+ # If the policy template type is profiles, then the input must be otelcol.
+ - if:
+ properties:
+ type:
+ const: profiles
+ then:
+ properties:
+ input:
+ enum:
+ - otelcol
icons:
$ref: "../integration/manifest.spec.yml#/definitions/icons"
screenshots:
@@ -152,7 +163,11 @@ versions:
- op: remove
path: "/properties/policy_templates/items/properties/template_paths" # removes template_paths field for policy templates
- op: remove
- path: "/properties/policy_templates/items/oneOf" # removes oneOf constraint for template_path/template_paths
+ path: "/properties/policy_templates/items/allOf/0/oneOf" # removes oneOf constraint for template_path/template_paths
+ - op: remove
+ path: "/properties/policy_templates/items/allOf/1" # removes oneOf constraint for profiles stream type
+ - op: remove
+ path: "/properties/policy_templates/items/allOf" # no longer needed, there are no constraints
- op: add
path: "/properties/policy_templates/items/required"
value:
Probably the JSON patch could be just removing the /properties/policy_templates/items/allOf
There was a problem hiding this comment.
this new input package type profiles just is intended to be used in OTel input packages
This is correct.
There was a problem hiding this comment.
Great suggestion, I've included that in the latest update
There was a problem hiding this comment.
Thanks @MichelLosier !
Just wondering if there is something that could be assert as a Then in the compliance test.
we could setup a new assertion for compliance tests that validates the expected otel config.
Could it be added an assertion that downloads the policy and ensures that there is some pipeline with the prefix profiles?
Any other assertion that could be added here @jsoriano ?
service:
pipelines:
profiles/profilingreceiver-otelcol-otelcol-good_input_profiles-profilingreceiver:
receivers:
- >-
profiling/profilingreceiver-otelcol-otelcol-good_input_profiles-profilingreceiver
processors:
- >-
transform/profilingreceiver-otelcol-otelcol-good_input_profiles-profilingreceiver-routing
There was a problem hiding this comment.
Sure thing! Added a new compliance test scenario to check for otel pipeline by name in the generated input templates for the package
14b16e5
9b25279
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/changelog.yml`:
- Around line 36-42: The two changelog items with descriptions "Add support for
auto_expand_replicas, max_result_window, and refresh_interval index settings in
data stream templates." and "Add support for `profiles` policy template type in
input packages" are placed incorrectly near the top of the 3.6.0-next block;
move those two entries to the bottom of the existing 3.6.0-next list so new
entries follow the guideline of appending to the end of the current version’s
changes, preserving their fields (description, type, link) and formatting
exactly as in spec/changelog.yml.
In `@spec/integration/data_stream/manifest.spec.yml`:
- Around line 266-271: The manifest adds three new 3.6.0-next index settings
(auto_expand_replicas, max_result_window, refresh_interval) and their
definitions (index_template_setting_auto_expand_replicas,
index_template_setting_max_result_window,
index_template_setting_refresh_interval) but lacks <3.6.0 patch removals; add a
patch for versions <3.6.0 that first removes the property references
(auto_expand_replicas, max_result_window, refresh_interval) from the relevant
object schemas and then removes the corresponding definition entries
(index_template_setting_auto_expand_replicas,
index_template_setting_max_result_window,
index_template_setting_refresh_interval), following the same pattern used
elsewhere (apply same removals in the other mentioned ranges as well).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
code/go/pkg/validator/validator_test.gocompliance/features/basic.featurespec/changelog.ymlspec/input/manifest.spec.ymlspec/integration/data_stream/manifest.spec.ymltest/packages/bad_input_profiles_non_otel/agent/input/input.yml.hbstest/packages/bad_input_profiles_non_otel/changelog.ymltest/packages/bad_input_profiles_non_otel/docs/README.mdtest/packages/bad_input_profiles_non_otel/manifest.ymltest/packages/good_input_profiles/agent/input/input.yml.hbstest/packages/good_input_profiles/changelog.ymltest/packages/good_input_profiles/docs/README.mdtest/packages/good_input_profiles/manifest.ymltest/packages/good_v3/data_stream/foo/manifest.yml
6e57f5a to
2c42d1b
Compare
| apiPackagePolicyPath = "/api/fleet/package_policies" | ||
| apiAgentPolicyPath = "/api/fleet/agent_policies" | ||
| apiPackagePolicyPath = "/api/fleet/package_policies" | ||
| apiInputTemplatesPath = "/api/fleet/epm/templates/%s/%s/inputs" |
There was a problem hiding this comment.
I think the results of this API returns the inputs information even when the package is not installed , just present in the package-registry or installed but without creating any policy.
For instance a request like this :
https://localhost:5601/api/fleet/epm/templates/nginx_otel_input/0.2.0/inputs
would return the input information even without installing nor creating any policy for that nginx_input_otel package.
So, not sure if this would be validating that the right policy (e.g. pipeline for the profiling is created) is created after the Given and And statements, WDYT ?
At least, defining this compliance test just as it was before validates that the policy is created (but without validating anything about it):
@3.6.0
Scenario: OTel input package with profiles type can be installed
Given the "good_input_profiles" package is installed
And a policy is created with "good_input_profiles" package, "0.0.1" version, "profilingreceiver" template, "profilingreceiver" input, "otelcol" input type and dataset "spec.otel_input_test"
So, I think it would be better to just keep the previous implementation and revert the latest changes.
Thanks for trying this approach in any case @MichelLosier !!
There was a problem hiding this comment.
I think the results of this API returns the inputs information even when the package is not installed
Ah good catch! No problem, I'll go ahead and remove it.
| @3.6.0 | ||
| Scenario: OTel input package with profiles type can be installed | ||
| Given the "good_input_profiles" package is installed | ||
| And a policy is created with "good_input_profiles" package, "0.0.1" version, "profilingreceiver" template, "profilingreceiver" input, "otelcol" input type and dataset "spec.otel_input_test" No newline at end of file |
There was a problem hiding this comment.
It would be helpful to add here a comment to indicate that packages using profiles are expected to not create any index template. And a link to probably this comment:
… for profiles packages
💚 Build Succeeded
History
|
What does this PR do?
profilesas a policy_template type for input packagesauto_expand_replicas,max_result_window, andrefresh_intervalRequires the kibana changes to add support for
profilesotel transforms: elastic/kibana#254090Why is it important?
Adds spec support for an OTel Profiling integration
Checklist
test/packagesthat prove my change is effective.spec/changelog.yml.Related issues